Add a wave propagation ripple effect to the website navbar#4150
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the simple navbar ripple animation with a more complex physics-based wave simulation, involving local 'lift' bumps and propagating wave pulses. The implementation includes a sampling-based curve generation using Catmull-Rom splines and updates to the SVG structure and SCSS for better visual fidelity. A significant performance issue was identified in the animation loop: frequent calls to getBoundingClientRect and getComputedStyle cause layout thrashing. It is recommended to cache these layout-dependent values and only update them on window resize to ensure a smooth frame rate.
There was a problem hiding this comment.
2 issues found across 3 files
Confidence score: 4/5
- This PR looks safe to merge overall: the reported issues are moderate and mostly around quality/performance rather than a clear functional break.
- The most impactful item is in
website/static/js/navbar.js:getComputedStyleis being called every animation frame insetRipples, which can cause avoidable rendering overhead and animation jank; caching resize-dependent values would reduce risk. - The
website/sass/base.scssfinding is a PR-title policy/enforcement concern (imperative mood), so it appears process-related rather than a runtime regression in the changed code. - Pay close attention to
website/static/js/navbar.js- repeated per-frame style reads in ripple animation may hurt UI performance under load.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
4b7a823 to
847b8e9
Compare
15fcaac to
d5f0140
Compare
8ff1338 to
0c2892c
Compare
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Now behaving just as I'd always envisioned it from the beginning!